-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix OpMergeParagraph merging paragraphs with empty annotations #877
Fix OpMergeParagraph merging paragraphs with empty annotations #877
Conversation
* @return {?Node} parent of targetNode | ||
*/ | ||
function removeUnwantedNodes(targetNode, shouldRemove) { | ||
function removeUnwantedNodes(targetNode, shouldRemove, shouldSkip) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of the shouldSkip
makes this look more and more like it should follow the existing TreeWalker pattern of using return values rather than boolean filters (e.g., return NodeFilter.REJECT = delete, NodeFilter.SKIP = skip, NodeFilter.ACCEPT = keep).
The point of ambiguity with the interface here is what happens if shouldSkip
& shouldDelete
are both true?
You could easily fix that in the API documentation, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I see your point. Indeed would make this method more flexibel if the callee has control about how shouldSkip & shouldDelete relate to each other. Will give it a try.
Other than that small thing, the change looks fine to me. The build-bot hasn't picked this up? |
test this please |
Build failed. |
test this please |
1 similar comment
test this please |
Build succeeded. |
Buildbot needed to be fed with enough working memory again... @peitschie So how do you like the other variant you proposed, now that you can see it in code? Myself like it slightly better than the first version, for the reason you gave. |
Yep... I like this a lot better too! Thanks for your rework. 👍 Please send this 🚢 off into the ocean that is webodf master. |
86a9e0c
to
45726eb
Compare
Build succeeded. |
Thanks for your good suggestion! |
…tations Fix OpMergeParagraph merging paragraphs with empty annotations
Before it removed the
<text:p/>
of empty annotations. Not nice.This fix makes sure embedded roots are not cleaned.